Add Structlog Handler as Instrumentation#4286
Add Structlog Handler as Instrumentation#4286JWinermaSplunk wants to merge 12 commits intoopen-telemetry:mainfrom
Conversation
e0317fa to
fab1022
Compare
| return _STD_TO_OTEL[levelno] | ||
|
|
||
|
|
||
| class StructlogHandler: |
There was a problem hiding this comment.
Could this be simplified by subclassing an existing type in structlog (which I don't know that well 🙂 )?
|
Thank you for undertaking this! |
fd07c37 to
ddae8b5
Compare
|
I think a rebase is needed. This PR is now reverting some unrelated docker-test fixes (this other PR) |
ef6ad9a to
ece1f64
Compare
Believe I went ahead and rebased, but feel free to let me know if it still looks off! |
|
Hey team, |
# Conflicts: # .github/workflows/test_0.yml # .github/workflows/test_1.yml # .github/workflows/test_2.yml # opentelemetry-contrib-instrumentations/pyproject.toml # Conflicts: # .github/workflows/core_contrib_test.yml
# Conflicts: # .github/workflows/test_1.yml # .github/workflows/test_2.yml # .github/workflows/test_3.yml
2648150 to
26c68f5
Compare
pmcollins
left a comment
There was a problem hiding this comment.
Hey Josh, this instrumentation seems to work fine, but I added some comments.
| return None | ||
|
|
||
|
|
||
| class StructlogHandler: |
There was a problem hiding this comment.
Would it be preferable to call this a StructlogProcessor since that's what structlog calls them? Also, below, we have processor = StructlogHandler(logger_provider=logger_provider).
There was a problem hiding this comment.
Fair point, I can adjust the line below, but I went with handler instead of processor to follow the logging handler from logging instrumentation, I believe. Thoughts?
There was a problem hiding this comment.
The LoggingHandler name makes sense because it's a subclass of logging.Handler. In this case the class doesn't extend anything -- it's just a callable that structlog considers a processor. I don't see a need to match terminology with that library, but won't block on this if you disagree.
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Not sure we want this because we could be classifying a local timestamp as UTC here. For example, one of the default structlog processors is TimeStamper(fmt="%Y-%m-%d %H:%M:%S", utc=False) which doesn't include a %z, so the emitted string doesn't include a timezone component, tzinfo is None, and we'd be then shifting the time to UTC.
| StructlogInstrumentor._processor = processor | ||
|
|
||
| # Wrap structlog.configure so that if user code calls it after | ||
| # instrumentation, the handler is re-inserted into the new chain. | ||
| StructlogInstrumentor._original_configure = structlog.configure |
There was a problem hiding this comment.
Can we convert these class variables to instance variables?
There was a problem hiding this comment.
I might be wrong, but isn't this the same behavior as existing instrumentations that use BaseInstrumentor and therefore would be protected by that singleton guard?
There was a problem hiding this comment.
Yeah, but I think variables should have the narrowest scope possible.
| # instrumentation, the handler is re-inserted into the new chain. | ||
| StructlogInstrumentor._original_configure = structlog.configure | ||
|
|
||
| def _patched_configure(**kwargs): |
There was a problem hiding this comment.
nit: since this is a closure I don't think you need the leading underscore.
| if StructlogInstrumentor._processor is None: | ||
| return |
There was a problem hiding this comment.
I'm not sure this test is necessary -- looks like the only way _processor can be None is if the user calls uninstrument without calling instrument, but seems like that's guarded against by the parent class. Which means the class variable can go away and patched_configure can just refer to _processor as a local (or just processor) since it's a closure.
| ) | ||
| kwargs["processors"] = processors | ||
| original = StructlogInstrumentor._original_configure | ||
| if original is not None: |
There was a problem hiding this comment.
Can original be None here? Might be an unnecessary test.
|
|
||
| # Get the log level and map to OTel severity | ||
| level_str = event_dict.get("level", "info") | ||
| levelno = _STRUCTLOG_LEVEL_TO_LEVELNO.get(level_str.lower(), 20) |
There was a problem hiding this comment.
Looks like 20 maps to INFO -- might be more appropriate to use Otel's UNSPECIFIED level that maps to 0. You may be able to use SeverityNumber.UNSPECIFIED in the API package.
| # Handle exception information | ||
| exc_info = event_dict.get("exc_info") | ||
|
|
||
| if exc_info is True: |
There was a problem hiding this comment.
Might want to add a comment here that is True is required to prevent matching on a non-empty tuple or exception which are also truthy. (Pycharm linter is flagging this)
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes open-telemetry/opentelemetry-python#2993
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.